Upgrade Font Awesome from 5.1.0 to 5.7.2 by Xenonym · Pull Request #713 · MarkBind/markbind

Xenonym · 2019-02-19T12:08:53Z

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [X] Enhancement to an existing feature

Fixes #510.

What is the rationale for this request?
With #680, Font Awesome icon names no longer need to be manually hardcoded to be used when referenced with the new colon syntax. As such, we can straightforwardly upgrade our version of Font Awesome by just replacing the CSS and font files. We can also use the @fortawesome/fontawesome-free NPM package to keep Font Awesome updated in the future.

What changes did you make? (Give an overview)

I installed @fortawesome/fontawesome-free@5.7.2.
I moved the Glyphicon asset files into asset/glyphicons/, to avoid having fonts/ (from Glyphicons) and webfonts/ (from Font Awesome) in the same folder.
I added Site.js#copyFontAwesomeAsset to copy Font Awesome files from the NPM package to asset/fontawesome/.
I fixed the behavior of the fs.copyAsync() mock to match its actual behaviour (details in 8e438cb)

Provide some example code that this change will affect:

- FA new icons: :fab-markdown: :fas-biohazard:

Testing instructions:

Add the above example code to a MarkBind site. It should render the new FA icons:

acjh · 2019-02-19T12:51:39Z

Why are glyphicons affected?

Xenonym · 2019-02-19T15:04:09Z

Why are glyphicons affected?

@acjh I shifted fonts/ to webfonts/ and edited the Glyphicon CSS files to match as the new FA files use webfonts/. Since we will update FA but not Glyphicons in the future, changing the directory structure to match FA's should be more efficient.

acjh · 2019-02-19T15:23:43Z

What's more efficient? Glyphicons and Font Awesome use separate files right?

Xenonym · 2019-02-19T15:36:00Z

What's more efficient? Glyphicons and Font Awesome use separate files right?

@acjh Right, efficient may not be the right choice of word. What I meant was, the new FA files use webfonts/, while Glyphicons uses fonts/. Since it would be confusing to have both webfonts/ and fonts/, I decided to consolidate both font folders. I chose to change the Glyphicons CSS files to refer to webfonts/ instead of changing the FA CSS files to use fonts/ since we will be likely to do FA upgrades in the future, and changing the Glyphicons CSS files once would be more "efficient" for upgrading than to change the FA CSS files to refer to fonts/ every time.

On the flip side, since Glyphicons are no longer in Bootstrap, it is unlikely that we would have to change out those files in the future as there is no longer a publicly available version of Glyphicons beyond what used to be in Bootstrap.

acjh · 2019-02-19T16:06:33Z

Let's not change the original paths.
https://maxcdn.bootstrapcdn.com/bootstrap/3.3.7/css/bootstrap.min.css

Also, shall we use npm to manage Font Awesome?
https://fontawesome.com/how-to-use/on-the-web/setup/using-package-managers

Xenonym · 2019-02-20T06:02:24Z

Let's not change the original paths.

👍 Might I suggest doing assets/fontawesome/ and assets/glyphicons? I feel that having both webfonts/ and fonts/ in the same folder will lead to confusion in the future.

Also, shall we use npm to manage Font Awesome?

This seems like a good idea, but we would need to change our asset logic to copy from both assets/ and the NPM package. Also, currently we bundle ~2MB of FA asset files with the package, but to use the NPM package would involve ~13MB, which is a not-insignificant size bump.

yamgent · 2019-02-21T07:31:08Z

This seems like a good idea, but we would need to change our asset logic to copy from both assets/ and the NPM package. Also, currently we bundle ~2MB of FA asset files with the package, but to use the NPM package would involve ~13MB, which is a not-insignificant size bump.

The size bump is only on the MarkBind application side right? When we build the website, we may not need the entire package, we can just copy their webfonts/ folder?

Using an npm package may also make upgrading the icons more convenient in the future.

Xenonym · 2019-02-21T14:26:06Z

The size bump is only on the MarkBind application side right? When we build the website, we may not need the entire package, we can just copy their webfonts/ folder?

Yes, that's right. The 13 MB is just an increase in size when a user installs MarkBind. However, built sites will still be the same size.

Using an npm package may also make upgrading the icons more convenient in the future.

Agreed. However, I was wondering if the benefit of this outweighs the increase in install size? (given that we don't use most of the Font Awesome package apart from the pre-built files)

yamgent · 2019-02-22T08:22:51Z

Agreed. However, I was wondering if the benefit of this outweighs the increase in install size? (given that we don't use most of the Font Awesome package apart from the pre-built files)

Hmm, I agree that it makes our app a bit more bloated. @Chng-Zhi-Xuan what do you think?

Chng-Zhi-Xuan · 2019-02-22T09:01:46Z

I think going forward to update Font Awesome automatically is necessary, especially if it can reduce manual labour.

MarkBind as a product is also operating under some assumptions of our users:

The author is using it on a computer
Has reasonable internet access

So the 13MB increase in install size is quite trivial given that a 1 minute 720P YouTube video is approximately 40MB already.

Xenonym · 2019-02-23T18:10:47Z

I shall try to see how to upgrade Font Awesome with the NPM package then!
Should I create another PR for that?

Chng-Zhi-Xuan · 2019-02-24T10:39:20Z

Should I create another PR for that?

Putting it in this PR is ok.

Xenonym · 2019-03-05T11:26:59Z

This PR has been updated to use Font Awesome via the NPM package!

yamgent · 2019-03-08T09:26:08Z

I will take a closer look when I get home at night, but generally I haven't spotted a big issue yet.

Meanwhile,

"Move glyphicon files to asset/glyphicons" commit should be wrapped at 72 characters. Also maybe clean the commit message a bit, by writing a standard paragraph rather than using a bullet point?

Font Awesome stores fonts in webfonts/, while Glyphicons stores fonts in fonts/. To avoid confusion between these two similarly named folders, let's shift Glyphicon files to asset/glyphicons/.

fs-extra-promise#copyAsync creates folders along the destination if they do not already exist. For example: fse.copyAsync('file.txt', 'does/not/exist/dest.txt') will succeed and create the folders "does", "not" and "exist". This is different from the behaviour of fs.copyFileSync, which raises an error as the destination folder does not exist. Thus, our test mocks for copyAsync are incorrect as they rely on the mock for copyFileSync. Let's create new mocks for copyAsync and copyDirAsync such that their behaviour matches that of the actual copyAsync.

Xenonym · 2019-03-08T17:34:08Z

@yamgent commit messages updated!

yamgent · 2019-03-10T04:59:05Z

@yamgent commit messages updated!

Nice work. 👍

Only one minor nit, for commit message in "Copy Font Awesome assets from NPM package", you can drop the word "Currently", it is implied.

For commit message in "Return promise directly from copyMarkbindAsset", we could have avoided using the uncommonly heard jargon, by directly explaining the problem with the anti-pattern (the original code is an exercise in futility), but the diff speaks for itself so I am open with leaving the message as it is now.

- Remove explicit promise construction anti-pattern: https://stackoverflow.com/a/23803744

We include Font Awesome assets by manually copying them to our own package. Since Font Awesome maintains an NPM package, we should copy Font Awesome assets from the NPM package assets for easier maintainence in the future. Let's remove the existing Font Awesome assets in the MarkBind directory and copy them from the NPM package. We should also copy them to _markbind/fontawesome, to prevent confusion with Glyphicon assets.

Chng-Zhi-Xuan

LGTM 👍

yamgent self-requested a review March 6, 2019 04:18

yamgent requested changes Mar 8, 2019

View reviewed changes

Comment thread src/Site.js

Comment thread __mocks__/fs-extra-promise.js Outdated

yamgent force-pushed the master branch from 7f35131 to 43a048d Compare March 8, 2019 15:01

Xenonym added 4 commits March 9, 2019 00:41

Add fontawesome-free@5.72

ea1aa1b

Move glyphicon files to asset/glyphicons

ee31f64

Font Awesome stores fonts in webfonts/, while Glyphicons stores fonts in fonts/. To avoid confusion between these two similarly named folders, let's shift Glyphicon files to asset/glyphicons/.

Fix typo in fs-extra-promise mock description

27511db

Xenonym added 2 commits March 11, 2019 02:47

Return promise directly from copyMarkbindAsset

388dfdc

- Remove explicit promise construction anti-pattern: https://stackoverflow.com/a/23803744

Chng-Zhi-Xuan approved these changes Mar 11, 2019

View reviewed changes

yamgent approved these changes Mar 11, 2019

View reviewed changes

yamgent added this to the v1.19.4 milestone Mar 11, 2019

yamgent merged commit 851b7a0 into MarkBind:master Mar 11, 2019

Xenonym deleted the assets/upgrade-fontawesome-5.7.2 branch March 11, 2019 08:09

Uh oh!

Conversation

Xenonym commented Feb 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acjh commented Feb 19, 2019

Uh oh!

Xenonym commented Feb 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acjh commented Feb 19, 2019

Uh oh!

Xenonym commented Feb 19, 2019

Uh oh!

acjh commented Feb 19, 2019

Uh oh!

Xenonym commented Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yamgent commented Feb 21, 2019

Uh oh!

Xenonym commented Feb 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yamgent commented Feb 22, 2019

Uh oh!

Chng-Zhi-Xuan commented Feb 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Xenonym commented Feb 23, 2019

Uh oh!

Chng-Zhi-Xuan commented Feb 24, 2019

Uh oh!

Xenonym commented Mar 5, 2019

Uh oh!

yamgent commented Mar 8, 2019

Uh oh!

Uh oh!

Uh oh!

Xenonym commented Mar 8, 2019

Uh oh!

yamgent commented Mar 10, 2019

Uh oh!

Chng-Zhi-Xuan left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Xenonym commented Feb 19, 2019 •

edited

Loading

Xenonym commented Feb 19, 2019 •

edited

Loading

Xenonym commented Feb 20, 2019 •

edited

Loading

Xenonym commented Feb 21, 2019 •

edited

Loading

Chng-Zhi-Xuan commented Feb 22, 2019 •

edited

Loading